perf: optimize date_parser / cast string to date (up to 2x faster)#4917
Conversation
andygrove
changed the title
date_parser / cast string to date (up to 2x faster)
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
First pass, thanks @andygrove!
| /// Days since 1970-01-01 for a proleptic Gregorian year/month/day, or `None` when the | ||
| /// combination is not a real calendar date. Unlike `NaiveDate::from_ymd_opt`, this accepts | ||
| /// any year that fits in `i64`. | ||
| fn ymd_to_epoch_day(year: i64, month: i64, day: i64) -> Option<i64> { |
There was a problem hiding this comment.
date_parser_test (native/spark-expr/src/conversion_funcs/string.rs:2712) never feeds a canonical-shape dddd-dd-dd string that is an invalid calendar date, so the branch where the fast path calls ymd_to_epoch_day and gets None is untested. The existing invalid-date cases (2020-010-01, 202, 2020-\r8) all fall through to the general parser because they are not 10-char dddd-dd-dd. A regression that made the fast path skip calendar validation would pass the current suite.
Suggested change: add to the invalid list in date_parser_test (which asserts None for Legacy/Try and is_err() for Ansi is wrong here, see finding 2, so put these in a Legacy/Try-only null assertion) canonical invalid dates and one valid boundary:
// invalid calendar dates in canonical yyyy-mm-dd form (exercise the fast path)
for date in &["2020-02-30", "2021-02-29", "2020-13-01", "2020-00-15", "2020-04-31", "2020-01-00"] {
for eval_mode in &[EvalMode::Legacy, EvalMode::Try, EvalMode::Ansi] {
assert_eq!(date_parser(date, *eval_mode).unwrap(), None);
}
}
// valid leap day through the fast path
assert_eq!(date_parser("2020-02-29", EvalMode::Legacy).unwrap(), Some(18321));There was a problem hiding this comment.
Added canonical-form invalid dates (2020-02-30, 2021-02-29, 2020-13-01, 2020-00-15, 2020-04-31, 2020-01-00) asserting None for all three eval modes, plus a valid leap-day case (2020-02-29 → 18321) that exercises the successful fast path.
…-mode; assert i32 fit Adds a group of canonical yyyy-mm-dd invalid calendar dates and a leap-day positive case to date_parser_test so a regression that skipped calendar validation in the fast path would fail. Rewrites the fast-path comment to describe the actual Comet behavior (Ok(None) treated as null in every eval mode by the caller) rather than an incorrect claim about Spark. Reinstates loud failure on the general-path cast via debug_assert! so a future weakening of the year range check does not silently wrap.
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
This LGTM after revision, thanks @andygrove!
2 participants
Which issue does this PR close?
N/A
Rationale for this change
Optimize existing expression.
What changes are included in this PR?
Replaced the per-row chrono NaiveDate construction and duration-to-epoch-day conversion with direct integer calendar validation plus the existing days_from_civil helper, and added a branch-free fast path for the canonical yyyy-mm-dd form that skips trimming, sign and digit-count checks.
How are these changes tested?
Existing tests.
Benchmark (criterion):
Full criterion output: